Skip to content

Conversation

@hawkingrei
Copy link
Member

@hawkingrei hawkingrei commented Dec 25, 2025

What problem does this PR solve?

Issue Number: close #65818

Problem Summary:

Analyze does not propagate cancellation context into RPC/NextRaw; killing the query can leave analyze workers blocked.

What changed and how does it work?

  • Pass SQLKiller-derived context into analyze workers and all V1 analyze column paths.
  • Replace context.TODO() with the propagated ctx in analyze V1 build/consume flow.
  • Add a failpoint-gated test to ensure analyze exits promptly after ctx cancellation.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 25, 2025
@hawkingrei
Copy link
Member Author

/retest

@codecov
Copy link

codecov bot commented Dec 25, 2025

Codecov Report

❌ Patch coverage is 75.59242% with 103 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.9980%. Comparing base (9b3495f) to head (f250aad).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #65249        +/-   ##
================================================
+ Coverage   77.7736%   77.9980%   +0.2243%     
================================================
  Files          2001       1922        -79     
  Lines        545596     533571     -12025     
================================================
- Hits         424330     416175      -8155     
+ Misses       119604     116954      -2650     
+ Partials       1662        442      -1220     
Flag Coverage Δ
integration 44.1638% <36.7298%> (-3.9972%) ⬇️
unit 76.4018% <75.5924%> (-0.0102%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 56.7974% <ø> (ø)
parser ∅ <ø> (∅)
br 48.8309% <ø> (-12.1598%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hawkingrei hawkingrei changed the title executor: fix analyze cannot be killed [WIP]executor: fix analyze cannot be killed Dec 25, 2025
@ti-chi-bot ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 25, 2025
@hawkingrei
Copy link
Member Author

/retest

@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 31, 2025
@hawkingrei hawkingrei changed the title [WIP]executor: fix analyze cannot be killed executor: fix analyze cannot be killed Jan 26, 2026
@ti-chi-bot ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-tests-checked labels Jan 26, 2026
@hawkingrei hawkingrei force-pushed the fix_cannot_kill_analyze branch from e93e052 to 253f0d7 Compare January 26, 2026 08:52
Copilot AI review requested due to automatic review settings January 26, 2026 08:52
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to make ANALYZE responsive to query kill/cancellation by propagating a SQLKiller-derived cancellation context into analyze workers and DistSQL/NextRaw paths, and adds a failpoint-based test to validate prompt exit on context cancellation.

Changes:

  • Add a SQLKiller-provided cancelable context (GetKillEventCtx) and propagate it through analyze execution paths.
  • Replace context.TODO() with a propagated ctx in several analyze V1/V2 build/consume flows and add ctx-aware worker loops.
  • Add a failpoint-gated unit test ensuring ANALYZE exits quickly after ctx cancellation.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
pkg/util/sqlkiller/sqlkiller.go Introduces kill-event context support for cancellation propagation.
pkg/util/sqlkiller/BUILD.bazel Adds dependency needed for new sqlkiller error usage.
pkg/executor/analyze.go Threads kill-derived ctx into analyze workers.
pkg/executor/analyze_col.go Propagates ctx into analyze V1 build/NextRaw flow.
pkg/executor/analyze_col_v2.go Adds ctx plumbing/cancellation to analyze V2 sampling workers and NextRaw usage.
pkg/executor/analyze_idx.go Adds ctx parameters through index analyze call chain and cancellation checks.
pkg/distsql/distsql.go Adds failpoint to block until ctx cancellation for testing.
pkg/executor/test/analyzetest/analyze_test.go Adds unit test validating analyze cancellation behavior.

@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2026
Copilot AI review requested due to automatic review settings January 26, 2026 10:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

statsHandle.FinishAnalyzeJob(results.Job, nil, statistics.TableAnalysisJob)
totalResult.results[results.Ars[0].Hist[0].ID] = results
case <-ctx.Done():
err = ctx.Err()
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On cancellation, this path sets err = ctx.Err(), which loses the cancellation cause from SQLKiller.GetKillEventCtx (set via cancelFn(errKilled)). Consider using context.Cause(ctx) (falling back to ctx.Err()) so the caller can differentiate SQL kill vs plain context cancellation.

Suggested change
err = ctx.Err()
err = context.Cause(ctx)
if err == nil {
err = ctx.Err()
}

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings January 26, 2026 13:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings January 26, 2026 13:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

*
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
*
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
*
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
@hawkingrei
Copy link
Member Author

/retest

1 similar comment
@YangKeao
Copy link
Member

/retest

@ti-chi-bot
Copy link

ti-chi-bot bot commented Jan 30, 2026

@hawkingrei: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-unit-test-next-gen f250aad link true /test pull-unit-test-next-gen

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@YangKeao
Copy link
Member

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/statistics release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analyze cannot be cancelled promptly

2 participants